Provide platform name to self-updater #541
Conversation
- Check current platform - Check if latest release has assets for the platform
praj-foss
added
the
Type: Maintenance
- Remove checks for 32-bit system - Add method to get asset download URL
| } else if (os == OperatingSystem.LINUX) { | ||
| platform = "linux"; | ||
| } else { | ||
| logger.warn("Current platform is unsupported: {}", System.getProperty("os.name")); |
There was a problem hiding this comment.
System.getProperty("os.name") - why not print out the os instead? Mabye update the toString to contain more information, if needed.
There was a problem hiding this comment.
That can be a quick fix. 👍 Since we're planning to get rid of that enum or maybe refactor it at least, it should contain just four constants: windows, mac, linux, and unsupported. So we might need to get it refactored while solving #542 too.
There was a problem hiding this comment.
Why would we need the unsupported constant?
There was a problem hiding this comment.
That'd be the default if someone somehow tries to run the launcher on anything other than the three supported platforms. We can use null of course, but I'd suggest avoiding that.
There was a problem hiding this comment.
I get what "unsupported" means ;)
I just don't get why we'd need a specific value for that...
Consider you want somebody to slice apples, peel potatoes and throw everything else you find in your kitchen cabinet away, which of the following would you rather write down and why?
A)
- open kitchen cabinet and do for each
- if "apple": slice
- if "potato": peel
- if "everything else": throw away
B)
- open kitchen cabinet and do for each
- if "apple": slice
- if "potato": peel
- else: throw away
There was a problem hiding this comment.
(B) seems natural 👍 (and that's what the else keyword was invented for)
| @@ -46,4 +51,14 @@ public String getTagName() { | |||
| public String getBody() { | |||
| return json.getString("body"); | |||
| } | |||
|
|
|||
| public String getDownloadUrl(String platform) { | |||
There was a problem hiding this comment.
Instead of putting utility methods with logic in here, just expose getAssets which returns a List<GithubAsset> (introduce new class). That class will provide means to getName() and getBrowserDownloadUrl().
There was a problem hiding this comment.
Easy to implement 👍. That's surely the most object-oriented approach, but let me ask if it's really necessary? Like if we are supposed to work with a single target asset (depending on the user's platform), why should we provide a list of assets or their names? 🙂
There was a problem hiding this comment.
I would like to keep all the classes in the github package as generic as possible, so that we could exchange them easily.
The current way we use the github api library is not ideal, as we are dealing with the plain JSON objects. In principle, those libraries offer interfaces similar to what is implemented in that package, so I'm preparing an easy switch.
This allows the self-updater to receive the name of the current platform, so it can download the correct assets from GitHub. It reuses the
OperatingSystemdetected duringLauncherInitTaskto know the platform name. Fixes #528.